Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: Set the -znostart-stop-gc flag for lld when testing #16682

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

torokati44
Copy link
Member

@torokati44 torokati44 commented Jun 11, 2024

Should fix CI failing with beta Rust (aside from the lints, for which there's #16679 and #16683).

Now that Rust beta also uses rust-lld by default on x86_64-unknown-linux-gnu, we can't use -Z as a workaround.

An alternative to #16448.
Also reverts #16390.
Referencing: dtolnay/linkme#94
For more info, see: https://lld.llvm.org/ELF/start-stop-gc

@torokati44 torokati44 force-pushed the linker-no-start-stop-gc branch from 92bca55 to 2156ad0 Compare June 11, 2024 15:54
@torokati44 torokati44 changed the title ci: Set the -znostart-stop-gc linker flag for testing ci: Set the -znostart-stop-gc flag for lld when testing Jun 11, 2024
@torokati44
Copy link
Member Author

torokati44 commented Jun 11, 2024

This conservative behavior works for existing code which does not take GC into fair consideration, but unnecessarily increases sizes for modern metadata section usage which desires precise GC.

GNU ld 2.37 added -z start-stop-gc to restore the traditional behavior ld.lld 13.0.0 defaults to -z start-stop-gc and supports -z nostart-stop-gc to switch to the conservative behavior.

However, since this is only an issue for testing (we use linkme to collect the test cases IIRC correction: known stubs), binary size shouldn't be much of an issue.

@torokati44
Copy link
Member Author

We may decide to wait for rustc to roll this change (switching to lld on linux by default) back from beta (dtolnay/linkme#88 (comment)) instead of merging this.
Or for linkme to fix this some other way with the change in rustc.

@danielhjacobs
Copy link
Contributor

See rust-lang/rust#126282 and rust-lang/rust#126278

@lqd
Copy link

lqd commented Jun 12, 2024

When using lld, this is likely to me to be the expected fix for linkme users, as I wrote in dtolnay/linkme#88 (comment) and the blog post announcing the change.

@torokati44
Copy link
Member Author

I see, thanks!

For the record, while looking into it, I had some other alternative ideas too, but I have no idea whether any of them are actually feasible. Such as:

  • Using the KEEP keyword in a linker script - linkme already has one for cortex-m, but I'm not sure it would be great to have one for x86_64 linux as well...
  • Adding a retain attribute to rustc, similar to used, which would affect the linker symbol...
  • Having linkme manually annotating the start/stop symbols to not be discarded...

@bjorn3
Copy link

bjorn3 commented Jun 12, 2024

Adding a retain attribute to rustc, similar to used, which would affect the linker symbol...

We have #[used(linker)] already. It is just unstable.

@torokati44
Copy link
Member Author

Considering that the next beta is coming out almost 6 weeks from now, I'm proposing merging this now, just to clean up CI, and unblock merging PRs without repo admin force.
As seen above, this will likely not be a bad fix long term either.

@Dinnerbone Dinnerbone force-pushed the linker-no-start-stop-gc branch from 2156ad0 to 3b8e03f Compare June 12, 2024 20:43
@Dinnerbone Dinnerbone merged commit 3a01dc5 into ruffle-rs:master Jun 12, 2024
16 of 17 checks passed
@lqd
Copy link

lqd commented Jun 14, 2024

Considering that the next beta is coming out almost 6 weeks from now

AFAICT the new beta is already out and lld is disabled there

@torokati44
Copy link
Member Author

Oh, right, great! Anyway, I think leaving that option won't really hurt...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants